-
Notifications
You must be signed in to change notification settings - Fork 26
claim isabled message fix #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 3 issues, and left some high level feedback:
- In
OldClaim, theuseEffectthat callsshowModal()has an empty dependency array but usesshowModal; consider addingshowModalto the dependency list or explicitly disabling the lint rule with a comment to make the intent clear. - The
Dialogreturned fromuseDisabledClaimingModalis created viauseMemoand returns a nestedDisabledClaimOverlaycomponent that closes overopen; this could be simplified by returning a memoized component viauseCallbackor an inline component withoutuseMemo, which would reduce indirection without changing behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `OldClaim`, the `useEffect` that calls `showModal()` has an empty dependency array but uses `showModal`; consider adding `showModal` to the dependency list or explicitly disabling the lint rule with a comment to make the intent clear.
- The `Dialog` returned from `useDisabledClaimingModal` is created via `useMemo` and returns a nested `DisabledClaimOverlay` component that closes over `open`; this could be simplified by returning a memoized component via `useCallback` or an inline component without `useMemo`, which would reduce indirection without changing behavior.
## Individual Comments
### Comment 1
<location> `src/pages/gd/Claim/OldClaim.tsx:86-88` </location>
<code_context>
return supportedChains.map((chainId) => chainNames[chainId] || `Chain ID: ${chainId}`).join(', ')
}, [supportedChains])
+ useEffect(() => {
+ showModal()
+ }, [])
+
// there are three possible scenarios
</code_context>
<issue_to_address>
**issue (bug_risk):** The useEffect that calls showModal should include showModal in its dependency array to avoid a stale closure and lint errors.
The effect calls `showModal` but uses an empty dependency array. Even though `showModal` is currently stable via `useCallback([])`, this creates future risk if that changes and will violate the React Hooks lint rule. Please either add `[showModal]` to the dependencies or explicitly disable the lint rule with a comment if this is intentionally a run-once effect.
</issue_to_address>
### Comment 2
<location> `src/pages/gd/Claim/OldClaim.tsx:62` </location>
<code_context>
- // const [, payload] = useFeatureFlagWithPayload('claim-feature')
- // const { enabled: claimEnabled = true, disabledMessage = '' } = (payload as any) || {}
+ const [, payload] = useFeatureFlagWithPayload('claim-feature')
+ const { /*enabled: claimEnabled = true,*/ disabledMessage = '' } = (payload as any) || {}
const { activeNetworksByFeature, activeChainFeatures } = useGoodDappFeatures()
const supportedChains = activeNetworksByFeature['claimEnabled'] || []
</code_context>
<issue_to_address>
**question (bug_risk):** Always showing the disabled claim modal on mount may ignore the feature-flag semantics that were previously intended.
With `claimEnabled` removed and `useEffect` always calling `showModal` on mount, the disabled overlay will still show even when the flag says the feature is enabled, or when there’s no payload (since `disabledMessage` defaults to an empty string). Consider restoring a guard around `showModal()` keyed off the feature flag and/or payload to preserve the original gating behavior.
</issue_to_address>
### Comment 3
<location> `src/pages/gd/Claim/useDisabledClaimModal.tsx:29` </location>
<code_context>
+
+ const showModal = useCallback(() => setOpen(true), [])
+
+ const Dialog = useMemo(() => {
+ const DisabledClaimOverlay = () => {
+ if (!open) return null
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the component-in-useMemo pattern with a useCallback-defined Dialog component that directly closes over state and props for a simpler, more idiomatic hook API.
You can simplify `useDisabledClaimingModal` by removing the component-in-`useMemo` pattern and just returning a stable `Dialog` component that closes over `open`/`message`.
Current pattern:
```ts
const Dialog = useMemo(() => {
const DisabledClaimOverlay = () => {
if (!open) return null
return (
<Box /* overlay */>
<Box /* modal */>
<DialogHeader />
<Box mt={4}>
<DialogBody message={message} />
</Box>
</Box>
</Box>
)
}
return DisabledClaimOverlay
}, [open, message])
```
This can be simplified without changing behavior:
```ts
export const useDisabledClaimingModal = (message: string) => {
const [open, setOpen] = useState(false)
const showModal = useCallback(() => setOpen(true), [])
const Dialog = useCallback(() => {
if (!open) return null
return (
<Box
position="absolute"
top={0}
left={0}
right={0}
bottom={0}
zIndex={50}
pointerEvents="auto"
bg="black:alpha.40"
display="flex"
alignItems="center"
justifyContent="center"
padding={4}
>
<Box bg="white" borderRadius={16} padding={5} maxW="343px" width="100%" shadow={9}>
<DialogHeader />
<Box mt={4}>
<DialogBody message={message} />
</Box>
</Box>
</Box>
)
}, [open, message])
return { Dialog, showModal }
}
```
Benefits:
- Removes the non-standard “useMemo returns a component” indirection.
- Keeps `Dialog` as a stable component reference (similar to the previous hook API).
- Maintains the new custom overlay layout and internal `open` state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| useEffect(() => { | ||
| showModal() | ||
| }, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): The useEffect that calls showModal should include showModal in its dependency array to avoid a stale closure and lint errors.
The effect calls showModal but uses an empty dependency array. Even though showModal is currently stable via useCallback([]), this creates future risk if that changes and will violate the React Hooks lint rule. Please either add [showModal] to the dependencies or explicitly disable the lint rule with a comment if this is intentionally a run-once effect.
| // const [, payload] = useFeatureFlagWithPayload('claim-feature') | ||
| // const { enabled: claimEnabled = true, disabledMessage = '' } = (payload as any) || {} | ||
| const [, payload] = useFeatureFlagWithPayload('claim-feature') | ||
| const { /*enabled: claimEnabled = true,*/ disabledMessage = '' } = (payload as any) || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (bug_risk): Always showing the disabled claim modal on mount may ignore the feature-flag semantics that were previously intended.
With claimEnabled removed and useEffect always calling showModal on mount, the disabled overlay will still show even when the flag says the feature is enabled, or when there’s no payload (since disabledMessage defaults to an empty string). Consider restoring a guard around showModal() keyed off the feature flag and/or payload to preserve the original gating behavior.
|
|
||
| const showModal = useCallback(() => setOpen(true), []) | ||
|
|
||
| const Dialog = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider replacing the component-in-useMemo pattern with a useCallback-defined Dialog component that directly closes over state and props for a simpler, more idiomatic hook API.
You can simplify useDisabledClaimingModal by removing the component-in-useMemo pattern and just returning a stable Dialog component that closes over open/message.
Current pattern:
const Dialog = useMemo(() => {
const DisabledClaimOverlay = () => {
if (!open) return null
return (
<Box /* overlay */>
<Box /* modal */>
<DialogHeader />
<Box mt={4}>
<DialogBody message={message} />
</Box>
</Box>
</Box>
)
}
return DisabledClaimOverlay
}, [open, message])This can be simplified without changing behavior:
export const useDisabledClaimingModal = (message: string) => {
const [open, setOpen] = useState(false)
const showModal = useCallback(() => setOpen(true), [])
const Dialog = useCallback(() => {
if (!open) return null
return (
<Box
position="absolute"
top={0}
left={0}
right={0}
bottom={0}
zIndex={50}
pointerEvents="auto"
bg="black:alpha.40"
display="flex"
alignItems="center"
justifyContent="center"
padding={4}
>
<Box bg="white" borderRadius={16} padding={5} maxW="343px" width="100%" shadow={9}>
<DialogHeader />
<Box mt={4}>
<DialogBody message={message} />
</Box>
</Box>
</Box>
)
}, [open, message])
return { Dialog, showModal }
}Benefits:
- Removes the non-standard “useMemo returns a component” indirection.
- Keeps
Dialogas a stable component reference (similar to the previous hook API). - Maintains the new custom overlay layout and internal
openstate.
Description
Summary by Sourcery
Restore and rework the claim-disabled modal so that users see a proper overlay with the configured disabled message on the legacy claim page.
Bug Fixes:
Enhancements:
Build: